Define chol for UniformScaling's #22633
Conversation
|
|
||
|
|
||
| ## Cholesky | ||
| function _chol!(J::UniformScaling, uplo) |
There was a problem hiding this comment.
This is following verbatim the chol(::Number) implementation.
There was a problem hiding this comment.
It is a trick to allow a recursive definition of chol. See
Line 75 in 71d7c16
! is not saying "will mutate", just "be aware, might mutate".
base/linalg/uniformscaling.jl
Outdated
|
|
||
| Compute the square root of a non-negative UniformScaling `J`. | ||
|
|
||
| # Example |
There was a problem hiding this comment.
Examples plural, no blank line after
There was a problem hiding this comment.
There was a problem hiding this comment.
there are pending pr's that are going to be changing all of the existing cases
There was a problem hiding this comment.
ok, then I only change that one instance in base/linalg/uniformscaling.jl
There was a problem hiding this comment.
yeah just change the one you're adding
|
Hm, on a related but orthogonal note to this, ERROR: StackOverflowError:
Stacktrace:
[1] chol(::Hermitian{UniformScaling{Int64},Array{UniformScaling{Int64},2}}) at ./linalg/cholesky.jl:188 (repeats 80000 times) |
|
Since the implementation is in |
|
Travis: Timeout. |
|
I would not like to move the tests, they integrate nicely into the file. On the other hand, the implementation could well be in linalg/cholesky.jl but the linalg/uniformscaling.jl is later in compilation order so I would not know how to rearrange this. |
|
I think that something like @testset "chol" begin
for T in (Float64, Complex128)
λ = T(4)
@test chol(λ*I) ≈ √λ*I
@test_throws LinAlg.PosDefException chol(-λ*I)
end
end
would integrate nicely into |
|
Actually, I let myself be convinced here. |
|
Bump. |
|
@fredrikekre are you happy with this as is? @tkelman ? |
* Define chol for UniformScaling's * Move test for chol(lambda*I) to test/linalg/uniformscaling.jl
| UniformScaling(c), info | ||
| end | ||
|
|
||
| chol!(J::UniformScaling, uplo) = ((J, info) = _chol!(J, uplo); @assertposdef J info) |
There was a problem hiding this comment.
What is the motivation for this method? Should fall in the same category as the now deprecated chol(::Number) IIUC, see #24498
There was a problem hiding this comment.
The function chol! is a chainable modifying array function and should do the right thing to immutable arrays. If !-versions of chainable array functions are defined for immutable arrays, then f!(g!(h!(copy(A())))) is a good way to write efficient and generic code which works for immutables and mutables (here ! allows f, g, h to use the copy of A as workspace where appropriate). In the same fashion, copy pushes through immutables and is defined for UniformScalings.
This came also up during the review by @tkelman of https://github.com/mschauer/Bridge.jl and is quite natural to define. There cannot be a CholFact because UniformScaling s are not AbstractArray s